-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement IEquatable<T> in record types #44994
Conversation
if (declaration.Kind == DeclarationKind.Record) | ||
{ | ||
var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T); | ||
baseInterfaces.Add(type.Construct(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this cause us to implement the interface "non-nullably?" Should we instead implement the interface with a nullable type argument, e.g. record Rec : IEquatable<Rec?>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see mention of IEquatable in the records spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter to IEquatable<T>.Equals()
is [AllowNull] T other
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see mention of IEquatable in the records spec.
Good catch, thanks. I've updated dotnet/csharplang#3552.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter to IEquatable.Equals() is [AllowNull] T other.
So it is, thank you! It might be good to add tests that demonstrate the synthesized "strongly typed Equals" methods accept null arguments without warnings or runtime errors. Since this test gap is not related to adding IEquatable implementation, will not block the PR over it.
if (declaration.Kind == DeclarationKind.Record) | ||
{ | ||
var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T); | ||
baseInterfaces.Add(type.Construct(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseInterfaces.Add [](start = 16, length = 18)
What if the interface already specified explicitly? #Closed
@@ -319,6 +319,12 @@ internal override ImmutableArray<NamedTypeSymbol> GetDeclaredInterfaces(ConsList | |||
} | |||
} | |||
|
|||
if (declaration.Kind == DeclarationKind.Record) | |||
{ | |||
var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T); [](start = 27, length = 73)
Please properly handle missing and otherwise bad types. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use-site errors are already reported for interfaces. See RecordTests.IEquatableT_09
. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use-site errors are already reported for interfaces. See RecordTests.IEquatableT_08.
Do we understand where and why that error is reported?
In reply to: 437852277 [](ancestors = 437852277)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SourceNamedTypeSymbol.InterfacesNoUseSiteDiagnostics()
adds use-site diagnostics for interfaces to Compilation.DeclarationDiagnostics
. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SourceNamedTypeSymbol.InterfacesNoUseSiteDiagnostics() adds use-site diagnostics for interfaces to Compilation.DeclarationDiagnostics.
I do not see anything there explicitly tasked to check use-site error on the implemented interfaces. Either we should find the place that explicitly fulfills the task, or we should do that here explicitly. Getting duplicated doesn't bother me at all.
In reply to: 438259982 [](ancestors = 438259982)
if (declaration.Kind == DeclarationKind.Record) | ||
{ | ||
var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T); | ||
baseInterfaces.Add(type.Construct(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseInterfaces.Add [](start = 16, length = 18)
We should make sure we test error scenarios around this interface. Places like that usually try to locate matching syntax in the base list. Obviously, in this case there is no syntax to find. We should make sure compiler doesn't crash. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found any use of GetBaseListOpt
, FindBaseRefSyntax
, FirstDeclarationWithExplicitBases
that doesn't check the return type in the case of interfaces. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found any use of GetBaseListOpt, FindBaseRefSyntax, FirstDeclarationWithExplicitBases that doesn't check the return type in the case of interfaces.
It looks like SourceNamedTypeSymbol.GetCorrespondingBaseListLocation can return null in our scenario. Consequently SourceMemberContainerTypeSymbol.GetImplementsLocation can return null. I see a number of call sites where result of calling this API is dereferenced or passed as a location for diagnostics. I suggest that we either confirm that those call sites are not reachable in our scenario, or change them to use one of the two helpers that already handle null result.
In reply to: 438274756 [](ancestors = 438274756)
var comp = CreateCompilation(source); | ||
comp.VerifyDiagnostics(); | ||
|
||
AssertEx.Equal(new[] { "System.IEquatable<A<T>>" }, comp.GetMember<NamedTypeSymbol>("A").AllInterfacesNoUseSiteDiagnostics.ToTestDisplayStrings()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AllInterfacesNoUseSiteDiagnostics [](start = 101, length = 33)
What do we return from Interfaces API?
#Closed
public virtual bool Equals(B other) => false; | ||
}"; | ||
var comp = CreateCompilation(source); | ||
comp.VerifyDiagnostics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comp.VerifyDiagnostics(); [](start = 12, length = 25)
Please run the code and observe expected methods called. Otherwise, how do we know that right methods are going to be hooked up? #Closed
record B : A<object>, IEquatable<A<object>>, IEquatable<B> | ||
{ | ||
bool IEquatable<A<object>>.Equals(A<object> other) => false; | ||
bool IEquatable<B>.Equals(B other) => false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool IEquatable.Equals(B other) => false; [](start = 4, length = 44)
Is this going to be a valid code without explicit IEquatable<B>
in the base list? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logged #45026.
record B : A<object>, IEquatable<A<object>>, IEquatable<B> | ||
{ | ||
public override bool Equals(A<object> other) => false; | ||
public virtual bool Equals(B other) => false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public virtual bool Equals(B other) => false; [](start = 4, length = 45)
Please add a test for a declaration like that, but without explicit IEquatable<B>
in the base list. #Closed
record B : A<object>, IEquatable<A<object>>, IEquatable<B> | ||
{ | ||
public override bool Equals(A<object> other) => false; | ||
public virtual bool Equals(B other) => false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public virtual bool Equals(B other) => false; [](start = 4, length = 45)
Please also add a test for a scenario when this method is
- not public
- not virtual #Closed
public void IEquatableT_01() | ||
{ | ||
var source = | ||
@"record A<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
record A [](start = 2, length = 11)
Please add a test for scenario when IEquatable<T>
has unexpected API in it. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IEquatableT_12
Done with review pass (iteration 1) #Closed |
if (declaration.Kind == DeclarationKind.Record) | ||
{ | ||
var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T).Construct(this); | ||
if (baseInterfaces.IndexOf(type, SymbolEqualityComparer.IgnoringNullable) < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymbolEqualityComparer.IgnoringNullable [](start = 49, length = 39)
I think we should ignore all aspects here #Closed
var compB = CreateCompilation(new[] { sourceB, IsExternalInitTypeDefinition }, references: new[] { refA }, parseOptions: TestOptions.RegularPreview); | ||
compB.VerifyDiagnostics( | ||
// (1,32): error CS8864: Records may only inherit from object or another record | ||
// record B(object P, object Q) : A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a record with explicit interface specified. Also, please assert Interfaces for all of them #Closed
if (declaration.Kind == DeclarationKind.Record) | ||
{ | ||
var type = DeclaringCompilation.GetWellKnownType(WellKnownType.System_IEquatable_T).Construct(this); | ||
if (baseInterfaces.IndexOf(type, SymbolEqualityComparer.AllIgnoreOptionsPlusNullableWithUnknownMatchesAny) < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AllIgnoreOptionsPlusNullableWithUnknownMatchesAny [](start = 72, length = 49)
This doesn't ignore all aspects #Closed
bool IEquatable<R>.Equals(R other) => false; | ||
}"; | ||
var comp = CreateCompilation(source); | ||
comp.VerifyDiagnostics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comp.VerifyDiagnostics(); [](start = 12, length = 25)
We should run and observe expected behavior. And also observe that we are not going to delegate to equality of the derived type when called through IEquatable<R>
interface. Not blocking for this PR
record B : A<int>, System.IEquatable<B>; | ||
"; | ||
var comp = CreateCompilation(source); | ||
comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); [](start = 12, length = 56)
It appears that we are not testing realistic scenario. I might be wrong, but I believe regular lookup isn't affected by this helper. We want the type be missing for real. #Closed
record B : A<int>, IEquatable<B>; | ||
"; | ||
var comp = CreateCompilation(source); | ||
comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comp.MakeTypeMissing(WellKnownType.System_IEquatable_T); [](start = 12, length = 56)
Same comment here. #Closed
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "IEquatable<B>").WithArguments("System.IEquatable`1").WithLocation(3, 20)); | ||
|
||
var type = comp.GetMember<NamedTypeSymbol>("A"); | ||
AssertEx.Equal(new[] { "System.IEquatable<A<T>>", "System.IEquatable<A<T>>[missing]" }, type.InterfacesNoUseSiteDiagnostics().ToTestDisplayStrings()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System [](start = 36, length = 6)
Howe do we know it is from System? #Closed
Diagnostic(ErrorCode.ERR_PredefinedTypeNotFound, "System.IEquatable<B>").WithArguments("System.IEquatable`1").WithLocation(2, 20)); | ||
|
||
var type = comp.GetMember<NamedTypeSymbol>("A"); | ||
AssertEx.Equal(new[] { "System.IEquatable<A<T>>", "System.IEquatable<A<T>>[missing]" }, type.InterfacesNoUseSiteDiagnostics().ToTestDisplayStrings()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssertEx.Equal(new[] { "System.IEquatable<A>", "System.IEquatable<A>[missing]" } [](start = 12, length = 86)
It looks like we have a duplicate type in the list. #Closed
"System.Type C.EqualityContract { get; }", | ||
"System.Object C.R { get; init; }", | ||
}; | ||
AssertEx.Equal(expectedMembers, actualMembers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public bool Equals(A other) => false; [](start = 4, length = 37)
This is a success case, should run and observe (not blocking for this PR)
AssertEx.Equal(new[] { "System.IEquatable<A<T>>[missing]" }, type.AllInterfacesNoUseSiteDiagnostics.ToTestDisplayStrings()); | ||
|
||
type = comp.GetMember<NamedTypeSymbol>("B"); | ||
AssertEx.Equal(new[] { "System.IEquatable<B>", "System.IEquatable<B>[missing]" }, type.InterfacesNoUseSiteDiagnostics().ToTestDisplayStrings()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.IEquatable", "System.IEquatable[missing]" [](start = 36, length = 54)
It looks like we have a duplicate type in the list. This is not blocking for this PR
Done with review pass (iteration 8) #Closed |
} | ||
|
||
[Fact] | ||
public void IEquatableT_16() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IEquatableT_16 [](start = 20, length = 14)
Did this scenario crash before the last commit? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because there is an explicit interface that is used as the fallback location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (iteration 9), assuming CI is passing
Fixes #44920